Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #635, Reduce use of uint32, add more OSAL typedefs #654

Merged
merged 2 commits into from
Nov 21, 2020
Merged

Fix #635, Reduce use of uint32, add more OSAL typedefs #654

merged 2 commits into from
Nov 21, 2020

Conversation

jphickey
Copy link
Contributor

Describe the contribution

Scrub all uses of uint32 type across OSAL, and replace most usage of this generic type with more appropriate, purpose-specific typedefs.

All use of uint32 to store an object size is now replaced with size_t.

Adds proper typedefs for:

  • osal_priority_t - used for indicating a task priority
  • osal_stackptr_t - for indicating a task stack pointer
  • osal_index_t - for indicating a table position/array index of all internal tables
  • osal_objtype_t - for indicating the type of an object

Fixes #635

Testing performed
Build and sanity test OSAL on supported platforms
Run all unit tests

Expected behavior changes
None. This is mainly a scrub of all APIs to use the proper type, but keeps it as uint32 most of the time, so it really shouldn't change anything. Except for sizes which are now size_t - so they will be 64 bits on a 64 bit platform.

System(s) tested on
Ubuntu 20.04
RTEMS 4.11

Additional context
This does expose another API problem with functions that return a size as an int32 (e.g. OS_read, OS_write, etc) .... as an int32 cannot represent the full range of possible values. We may need a type to correlate with POSIX ssize_t to address this.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey changed the base branch from main to integration-candidate November 12, 2020 16:31
@jphickey jphickey added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Nov 12, 2020
@astrogeco
Copy link
Contributor

CCB 2020-11-12 APPROVED

  • this improves maintenability
  • @acudmore will review
  • Similar change will be done on cFE
  • We'll have to think about how to do the similar fix to PSP

@jphickey
Copy link
Contributor Author

@astrogeco - note this PR does depend on PR nasa/cFE#1003 which is currently a draft.

I can either update this draft to a final rev or we can hold off on merging this until next cycle.

@jphickey
Fix nasa#635, add typedefs for osal stack and priority
2899dfe 
Adds typedefs for:

- osal_priority_t
- osal_stackptr_t
- osal_index_t
- osal_objtype_t
- osal_blockcount_t

Note that by using `uint8` as the priority type, all values are
valid and it is no longer possible to pass a value which is out
of range.  Checks/tests for this are no longer valid and would
cause compiler warnings.
@jphickey jphickey force-pushed the fix-635-osal-add-task-typedefs branch from 263ed34 to 2899dfe Compare November 16, 2020 18:45
@jphickey
Copy link
Contributor Author

Note - commit 2899dfe is a rebase to current "main" branch.

One thing to consider before final merge: the osal_blockcount_t is an alias to size_t here. There were a couple entries in the OS file abstraction where these were actually uint64 values - blocks in a disk file system. In this case technically even a 32-bit CPU can have a large file system >4G. But since this is a block count - not a byte count - that means even if size_t is 32 bits, it will still be able to describe a file system of at least 2TB (for 512 byte blocks) or more depending on block size. Most filesystems seem to use a blocksize of at least 4k so that is enough for 16TB volume without overflow.

Other alternatives:

  • make osal_blockcount_t to be 64 bit always.
  • make a separate osal_fs_off_t (file system offset) which is 64 bits and use this in the FS layer.

(In general I prefer the simplicity of using the same type everywhere but if large filesystems on 32 bit platforms are a thing, this might not be feasible)

@jphickey
Update nasa#635, type corrections to avoid conversion
9c081dc 
Additional type corrections to avoid implicit sign
and width conversions.
This was referenced Nov 17, 2020
Closed
Merged
@astrogeco astrogeco removed the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Nov 18, 2020
@astrogeco astrogeco mentioned this pull request Nov 18, 2020
Merged
@astrogeco astrogeco merged commit 5db9bdb into nasa:integration-candidate Nov 21, 2020
astrogeco added a commit to astrogeco/cFS that referenced this pull request Nov 21, 2020
@astrogeco
Add nasa/osal#654
b66ef62
astrogeco added a commit to astrogeco/cFS that referenced this pull request Nov 23, 2020
@astrogeco
Add nasa/osal#654
a47d6ab
@ghost ghost mentioned this pull request Nov 24, 2020
Merged
@astrogeco astrogeco mentioned this pull request Dec 1, 2020
Merged
@jphickey jphickey deleted the fix-635-osal-add-task-typedefs branch December 3, 2020 17:26
@skliper skliper linked an issue Dec 9, 2020 that may be closed by this pull request
OS_statvfs_t types do not match corresponding POSIX types #570
Closed
This was referenced Dec 9, 2020
Closed
Closed
@skliper skliper linked an issue Dec 9, 2020 that may be closed by this pull request
FIleSize in os_fstat_t is uint32, should be 64bit friendly #572
Closed
@skliper skliper added this to the 6.0.0 milestone Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Caelum
3 participants
@jphickey @astrogeco @skliper